Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: Remove support for NVDIMM namespaces #5353

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

poncovka
Copy link
Contributor

All additional support for NVDIMM is being deprecated and removed, especially the support for the namespace reconfiguration. However, namespaces configured in the block/storage mode can be still used for installation.

The nvdimm kickstart command is deprecated and will be removed in future releases.

Depends on: pykickstart/pykickstart#469
Blocks: storaged-project/blivet#1172

Resolves: #INSTALLER-3766

@poncovka poncovka added the f40 label Nov 28, 2023
@poncovka poncovka force-pushed the master-remove_nvdimm_support branch from 92ab31c to 4aefc02 Compare November 28, 2023 14:56
@poncovka poncovka marked this pull request as draft November 28, 2023 14:56
@poncovka poncovka added the blocked Don't merge this pull request! label Nov 28, 2023
@poncovka
Copy link
Contributor Author

@vojtechtrefny I am able to select a NVDIMM device in the fsdax mode for the installation. It just won't boot. I guess that is the expected behaviour, right? I have used the Blivet changes as well for testing.

nvdimm_without_support

nvdimm_without_support_out

@vojtechtrefny
Copy link
Contributor

@vojtechtrefny I am able to select a NVDIMM device in the fsdax mode for the installation. It just won't boot. I guess that is the expected behaviour, right?

Yes, device mapper doesn't work with the fsdax mode (only with the sector mode). I'd say this is expected and we should just ignore it.

@vojtechtrefny
Copy link
Contributor

@vojtechtrefny I am able to select a NVDIMM device in the fsdax mode for the installation. It just won't boot. I guess that is the expected behaviour, right?

Yes, device mapper doesn't work with the fsdax mode (only with the sector mode). I'd say this is expected and we should just ignore it.

We could add pmemN devices (but not pmemNs devices (sector mode namespaces)) to the ignored list to avoid this.

Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, but I didn't stop to think what might be missing...

@poncovka
Copy link
Contributor Author

We could add pmemN devices (but not pmemNs devices (sector mode namespaces)) to the ignored list to avoid this.

Done.

All additional support for NVDIMM is being deprecated and removed, especially
the support for the namespace reconfiguration. However, namespaces configured
in the block/storage mode can be still used for installation.

The `nvdimm` kickstart command is deprecated and will be removed in future
releases.

Resolves: #INSTALLER-3766
Don't allow to select devices that cannot be used for the installation.
For example, NVDIMM in the fsdax mode prevents the system from booting.
@poncovka poncovka force-pushed the master-remove_nvdimm_support branch from ebb40fb to 5da3b57 Compare November 29, 2023 14:11
@poncovka poncovka marked this pull request as ready for review November 29, 2023 14:11
@poncovka
Copy link
Contributor Author

/kickstart-test --testtype smoke

Copy link
Contributor

@vojtechtrefny vojtechtrefny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@poncovka poncovka removed the blocked Don't merge this pull request! label Nov 29, 2023
@poncovka poncovka merged commit 7ee760e into rhinstaller:master Nov 30, 2023
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants